-
Notifications
You must be signed in to change notification settings - Fork 203
Add a project version to the build. #887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this but I think there is no need for a separate library, unless you have plans for using it elsewhere later?
But since you've already put in the effort to add a new library I don't think we necessarily need to simplify this down to a single header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny formatting nit otherwise LGTM
There have already been significant changes since version 0.7 (including adding the new config system), so I wonder if we'd be better off calling it 0.8.0-dev or something along those lines. See #109 for lots of discussion about a version string. |
89ba1b6
to
0876fb4
Compare
cmake/project_version.cmake
Outdated
) | ||
|
||
if (git_error) | ||
message(FATAL_ERROR "Failed to run git describe: ${git_error}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means you can no longer build the downloaded archive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means you can no longer build the downloaded archive.
Should be fixed now.
ffeffce
to
b046baa
Compare
Unfortunately, a "-dev" suffix doesn't quite work with CMake's semver check, nor does |
@@ -0,0 +1,45 @@ | |||
# Set a default version for archive builds that do not have | |||
# git history. Increment this appropriately at tag-and-release time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have to do this anyway is there much benefit to using git describe
?
I wonder if it's simpler just to update it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake/project_version.cmake
Outdated
set(archive_build_version "0.7.0") | ||
|
||
# Sets GIT_EXECUTABLE | ||
find_package(Git REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably shouldn't be required? If git isn't found just use the archive_build_version.
Uses `git describe` to get the version according to git, and sanitizes it for CMake project function for potential future use with CPack. Adds an option to the c_emulator to report the (git) version. This requires the github checkout action to fetch a clean history at least upto the last tag. For now, fetch all history. For builds of a downloaded archive without git history, use a specified version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love having two different variants of the version string, but seems like the best option for now and worth getting in.
The last release on github used 0.7, so the version is set to 0.7.0.
Add an option to the c_emulator to report the version.